Add copying to clipboard using OSC52#974
Conversation
9bc259d to
81aea1d
Compare
|
@joshka would love your input on this |
joshka
left a comment
There was a problem hiding this comment.
General support for the idea with a few questions / thoughts. terminal seems like the right module for this unless you were to add a clipboard module, but there's only one command likely to end up there, so that feels like probably a bit unnecessary.
ab8c54f to
16bc099
Compare
9131b92 to
626768a
Compare
joshka
left a comment
There was a problem hiding this comment.
Lots of extra comments after reading the docs more and giving this a deeper review. Apologies that my first review was fairly surface level in comparison.
At a high level, I support the addition of this. To summarize my thoughts across the various comments raised here, I have some concern around what level of abstraction Crossterm should provide for this. I don't have any immediately concrete suggestions though, so looking for some back and forth of ideas before settling on the right approach. Feel free to shoot down any of the perspectives I've thrown here with your viewpoints - you're likely just as much or more of an expert on these than I am.
Regarding the paste side of this (passing ? as the data). It seems like it could be worth adding this usage of OSC 52 at the same time as adding this feature. WDYT?
If the second parameter is a ? , xterm replies to the host
with the selection data encoded using the same protocol. It
uses the first selection found by asking successively for each
item from the list of selection parameters.
Also there's a clear clipboard function - worth worrying about?
If the second parameter is neither a base64 string nor ? ,
then the selection is cleared.
|
Hi @joshka,
I appreciate this very much. Thank you for taking the time. (Also, I know the feeling of having to double down on a review very well. No worries. :) )
I've had these, too. This whole change feels odd as clipboard support breaks a barrier in so far as that it is very different from the usual presentation and input mechanisms supplied by terminals and in this case crossterm. It is only really relevant to applications as a means for copying from remote hosts, where the only line is a TTY. Thus, while odd, signalling the copy operation in-band is the go-to solution.
I don't have much to add to my previously stated opinion. There is essentially no need for paste (as pasting can be done through the terminal emulator and normal input) and normalizing its use is a huge security concern. You can't even use it to cross-check OSC52, as some terminal emulators disable OSC52 paste while having OSC52 copy enabled. (And after this has been merged I'll probably try to get alacritty's behavior into more emulators. I was quite surprised by how many emulators actually allow paste.) (I agree that there is a small impedance mismatch. If I run my favorite editor and if that were to use crossterm on a remote host, I can't just type My suggestion to consolidate this would be to actually move the clipboard code into a separate
I guess you're asking because a However, it is true that for clipboard protocols, there generally is a difference between being cleared and containing an empty string. That being said, I'm not sure how to address this given OSC52's specification. A wider API would only be relevant if there is a plan to support other clipboard protocols (like wayland, X.org, winapi, whatever macos is doing and whatever we want to implement ourselves) in the future. My view as a new contributor is that this is not in crossterm's scope. If you disagree, we should discuss this, though. Frankly, I do see a potential use case for copying a text/uri-list that contains sftp-URIs for pasting into your favorite file browser for file download through SSH. The only limit here is that associating a MIME type is not supported by OSC52. That does not really keep us from widening the interface to something resembling I'll respond to the other items in line or implement them. Cheers and thanks again, Johannes |
Sounds good - this also simplifies the feature flag checks to just include the entire module.
Your logic there seems sound to me. |
40ed098 to
90f1898
Compare
|
So, here's a new API that is a bit wider but offers and prioritizes simple constructors. I guess there will be criticism, I'm still open to any changes! Commands are now primarily constructed using Now this covers the whole OSC52 copy feature set, I guess, without too many visible complications. @joshka: Might I ask you to have another look at this if you can spare the time? Thank you, also for your excellent feedback in the previous two rounds! |
044c616 to
ab0fd66
Compare
|
Just FYI, pasting is best supported with permissions, just as it is on the web. And copy/paste for different mimetypes is implemented using the OSC 5522 protocol, documented at: |
joshka
left a comment
There was a problem hiding this comment.
nits and bikeshedding comments mainly. I'm fine with the approach here in general. Happy to take it as is or let you fix up which parts you think are relevant here.
There was a problem hiding this comment.
As a non-blocking idea, what about making this more interactive rather than command driven to make it clear that the behavior is part of a standard part of an application flow. E.g.
- print instructions to press c/p/esc
- loop, accept keys
- print a message that says what happened (copied "foo" to the primary / clipboard0
Absolutely this is mostly gold plating the demo. But I think it makes it a little bit more obvious. Also, what about naming this clipboard for making it easier to discover (when users are browsing the repo the name is often the first thing they'll see.
There was a problem hiding this comment.
Maybe this would be a good addition when paste get's implemented to show a fully interactive clipboard roundtrip. I've renamed it to copy-to-clipboard.rs for clarity, though.
I find this tool actually quite helpful as is. It allows me to quickly check crossterm's implementation against different terminal emulators without needing further interaction.
src/clipboard.rs
Outdated
| /// use crossterm::clipboard::CopyToClipboard; | ||
| /// execute!(std::io::stdout(), CopyToClipboard::into_clipboard_from("foo")); | ||
| /// ``` | ||
| pub fn into_clipboard_from(content: T) -> CopyToClipboard<T> { |
There was a problem hiding this comment.
bikeshed: I think maybe to_clipboard / to_primary might be obvious enough names here. into_ gives greater connotations of conversion of the type into another type, which is not happening here. with_ would be the obvious builder style name, but that feels wrong here to me too.
There was a problem hiding this comment.
This is taking off some mental load, thanks. Naming has been a major pain in this patch and I'm still dissatisfied. Everything is quite clunky. I'd love to name CopyToClipboard just Copy, but has an obvious readability issue in Rust due to std::marker::Copy. I've converted it, without having any better suggestion, CopyToClipboard::to_clipboard... also is an odd one.
Hi @kovidgoyal. Thank you for your input on paste, I appreciate it! It seems that, at the moment, this is only implemented in kitty, right? |
|
On Wed, Mar 19, 2025 at 11:45:04PM -0700, Johannes Agricola wrote:
naseschwarz left a comment (crossterm-rs/crossterm#974)
> Just FYI, pasting is best supported with permissions, just as it is on the web. And copy/paste for different mimetypes is implemented using the OSC 5522 protocol, documented at: https://sw.kovidgoyal.net/kitty/clipboard/
Hi @kovidgoyal. Thank you for your input on paste, I appreciate it! It seems that, at the moment, this is only implemented in kitty, right?
Yes, so far as I know OSC 5522 is only supported in kitty, although, I
haven't really kept track.
However, since you are designing an API you might want to keep this in mind for the future.
|
01e65ca to
5751c03
Compare
|
Huge thanks again, @joshka. I've implemented all suggestions except for the more involved I also noticed that the CI isn't running clippy on all features and replaced my |
|
Worth noting that OSC52 support was previously rejected by the author due to new dependencies (the exact same ones this PR is using) |
There was a problem hiding this comment.
Yes indeed, previously dismissed the addition in the other PR, and similar concerns are raised here. In this PR we went for OSC52 support only without windows support, and compared the other PR this PR has done more work in documenting/testing the edge cases. On a second thought, with clear documentation and the feature behind a feature flag, I think its fine to have those features in the library given they are terminal protocols which crossterm aims to help simplifying access to.
LGTM, just one comment regarding the feature flag.
Cargo.toml
Outdated
| derive-more = ["dep:derive_more"] | ||
|
|
||
| ## Enables interacting with a host clipboard via [`clipboard`](clipboard/index.html) | ||
| clipboard = ["dep:base64"] |
There was a problem hiding this comment.
Since its only osc52 perhaps we can name the feature flag to reflect that? Otherwise folks may think its a general clipboard feature which can have many kinds of implementations.
There was a problem hiding this comment.
Thanks for your time and the approval, @TimonPost. I'll gladly yield to renaming the flag to osc52, see my latest change.
Apart from this, the latest change to the MR is rebasing onto master and adding an entry to CHANGELOG.md. I hope that's fine.
5751c03 to
dd92f6b
Compare
Many terminal emulators support copying text to clipboard using ANSI OSC Ps; PT ST with Ps = 5 2, see https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Operating-System-Commands This enables copying even through SSH and terminal multiplexers.
1fe99be to
feaeb56
Compare
|
Thanks for your patience and work! |
Many terminal emulators support copying text to clipboard using ANSI OSC Ps; PT ST with Ps = 5 2, see https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Operating-System-Commands This enables copying even through SSH and terminal multiplexers. Co-authored-by: Naseschwarz <naseschwarz@0x53a.de>
Many terminal emulators support copying text to clipboard using ANSI OSC Ps; PT ST with Ps = 5 2, see
https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-Operating-System-Commands
This enables copying even through SSH and terminal multiplexers.
I'm not too sure whether you want this feature or whether
src/terminal.rsis the right place for it. I had just implemented it for gitui, which uses crossterm, and thought, it would be nice to have it upstream too.It does not come for free, as it adds a new dependency to base64 which is not yet required by any of crossterm's dependencies.